Skip to content

[SPARK-43309][SPARK-38461][CORE] Extend INTERNAL_ERROR with categories and add error class INTERNAL_ERROR_BROADCAST#40978

Closed
bozhang2820 wants to merge 2 commits intoapache:masterfrom
bozhang2820:spark-38461
Closed

[SPARK-43309][SPARK-38461][CORE] Extend INTERNAL_ERROR with categories and add error class INTERNAL_ERROR_BROADCAST#40978
bozhang2820 wants to merge 2 commits intoapache:masterfrom
bozhang2820:spark-38461

Conversation

@bozhang2820
Copy link
Contributor

What changes were proposed in this pull request?

This change is to extend INTERNAL_ERROR with categories, add INTERNAL_ERROR_BROADCAST as an example of it, and change exceptions created in the broadcast package to use that error class.

Why are the changes needed?

This is to better differentiate internal errors from different categories / areas / modules.

Does this PR introduce any user-facing change?

Yes. The exceptions created in the broadcast package will change to be of error class INTERNAL_ERROR_BROADCAST.

How was this patch tested?

Updated unit tests.

@github-actions github-actions bot added the CORE label Apr 27, 2023
@bozhang2820
Copy link
Contributor Author

The test failure in GitHub Actions "track allocated resources by taskId" seems to be unrelated to this change.

@bozhang2820
Copy link
Contributor Author

@cloud-fan, @srielau, could you take a look at this?

intercept[SparkException] { broadcast.value }
intercept[SparkException] { broadcast.unpersist(blocking = true) }
intercept[SparkException] { broadcast.destroy(blocking = true) }
val e1 = intercept[SparkException] { broadcast.value }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use the checkError util for testing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, we don't need to check error message here.

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 717ca6a Apr 28, 2023
@bozhang2820 bozhang2820 deleted the spark-38461 branch April 28, 2023 06:30
Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain, please, why didn't you introduce sub-classes? Like INTERNAL_ERROR.BROADCAST

@cloud-fan
Copy link
Contributor

@MaxGekk sub error class can't have dedicated sql state.

@MaxGekk
Copy link
Member

MaxGekk commented May 5, 2023

sub error class can't have dedicated sql state.

ok, but INTERNAL_ERROR_BROADCAST and INTERNAL_ERROR have the same sql state in the PR. Just in case, can't we override the sql state in sub-classes (are there any principal issues of implementing this?).

@bozhang2820
Copy link
Contributor Author

Another advantage is that having INTERNAL_ERROR_BROADCAST will make it possible to have subclasses for the broadcast package.

LuciferYang pushed a commit to LuciferYang/spark that referenced this pull request May 10, 2023
…s and add error class INTERNAL_ERROR_BROADCAST

### What changes were proposed in this pull request?

This change is to extend INTERNAL_ERROR with categories, add INTERNAL_ERROR_BROADCAST as an example of it, and change exceptions created in the broadcast package to use that error class.

### Why are the changes needed?

This is to better differentiate internal errors from different categories / areas / modules.

### Does this PR introduce _any_ user-facing change?

Yes. The exceptions created in the broadcast package will change to be of error class INTERNAL_ERROR_BROADCAST.

### How was this patch tested?

Updated unit tests.

Closes apache#40978 from bozhang2820/spark-38461.

Authored-by: Bo Zhang <bo.zhang@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants